-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
import current 9front rc #558
base: master
Are you sure you want to change the base?
Conversation
Perhaps you could squash all of this down into a single commit, but more importantly, could you outline the major changes from stock rc, and why importing this would be useful? |
I'm happy to squash the commits down, I just didn't think about it as I was going along. As for the why, 9front's rc contains a whole slew of bug fixes, lexer/interpreter fixes, globber fixes, memory leaks, heredoc fixes, along with a couple of extensions to the syntax. Another big point is recent work by cinap_lenrek to increase portability. To get a proper feel for everything that's been done, you'd really have to take a look at the 9front commit logs. It's quite a bit, and I'm not particularly qualified to comment on it. The rc currently in plan9port is fully functional, but with the work that's gone into 9front's rc, I think it's worth importing. I wouldn't push for it to be in 9legacy, because the goals there are different, but I think keeping plan9port up-to-date with an currently evolving Plan 9 is a good thing. I'll have no hard feelings if you decide this isn't something you want. I just thought I'd make it available. |
91ecfb8
to
be2ce6d
Compare
I think that's reasonable, but it's enough of a departure that it probably makes sense to get some more folks's input. In the meanwhile, rephrasing your commit message to read, |
90316f6
to
050f71c
Compare
Note -- this drops the recentish parser rewrite that p9p will want (and which we should probably import into 9front): 47d4646, 7d6a248, ff74f7c, 601e07b I think this PR should be updated to incorporate them (and the 9front rc should probably be changed to parse the same dialect of rc) before we land this. Also, as mentioned on grid, this code was run through an autoformatter -- don't do that! that makes any merges with future 9front changes will be hellish. |
Undoing the formatting now. I'll push again once that is complete. I hadn't realized p9p's parser had undergone as many changes as it had when I embarked on this. After discussion with @oridb, I agree that even after undoing the formatting changes, this shouldn't be merged without work on the parser. |
Oh. Yes, indeed. |
198e1de
to
9f420c3
Compare
Okay, this now uses the p9p parser with the 9front lexer updates. |
I think we're going to need more testing here. @oridb seemed to imply these changes (or at least, the basic grammar) was bound for 9front. Is that the case? |
Includes work to increase portability on *nix systems by cinap_lenrek, some extensions to the syntax, and multiple bug fixes to: - lexer/interpreter - globber - heredoc and much more.
Let's leave this open and figure it out. I'm not opposed to importing into 9front, but would want to talk it over and with cinap before I touch that. We've got a hackathon coming up (starting next week), so this is good timing. Let's decide what to do then. |
SGTM. I'm out of the country next week, but should have Internet access. |
Any update on this? I got this patch to compile on MacOS by adding
Entering RC also doesn't seem to be picking up or setting the path correctly.
What's weird is that Bash and Fish shell can execute scripts with '9' such as 9 ls and 9 lc. Any thoughts on how to resolve this issue? |
This patch still has seen effectively no testing. I'm going to try to take some time over the next couple weekends to re-port it back to 9front, and see if it can get merged there. I'm not sure what will happen here after that; presumably if it gets some use testing and lands on 9front we'll be able to get a merge here. MacOS problems:
NSIG is only defined on Plan9, or if you're on another OS and it isn't yet defined. I have no idea exactly where you're putting
I'll only need the output from |
Imports the most recent 9front rc.
Some of the diff is reformatting, and you might choose not to merge simply because of this.
If this an issue, I can work it over again minus that.